Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable attachment of binary files as charm resources #1147

Closed
wants to merge 1 commit into from

Conversation

nrobinaubertin
Copy link

Description

This enables the attachment of binary files as charm resources.

Fixes: #1000

@jujubot
Copy link
Contributor

jujubot commented Oct 8, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented Oct 8, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@dimaqq dimaqq requested review from Aflynn50 and gfouillet October 9, 2024 00:09
juju/model.py Outdated Show resolved Hide resolved
@dimaqq
Copy link
Contributor

dimaqq commented Oct 9, 2024

@nrobinaubertin this PR needs some kind of test.

ideally an integration test, or
a series of steps (this exact charm, this bootstrap, this deploy with this binary artifact)

Copy link

@gfouillet gfouillet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to more than dima.

  • Please add a test that would trigger the bug if not fixed
  • take care of all call site of _upload to ensure data type consistency.

@dimaqq
Copy link
Contributor

dimaqq commented Nov 8, 2024

Niels, I see that the PR where you needed this was closed.

Do you have the bandwidth to update this PR?

If not, I can maybe create an issue based on the PR and eventually someone else can pick this up. What you're fixing is basically a bug, as binary resources should be supported.

@dimaqq
Copy link
Contributor

dimaqq commented Nov 19, 2024

This PR needs to be rebased since #1186 has been merged.

@nrobinaubertin
Copy link
Author

nrobinaubertin commented Nov 20, 2024

I see that the PR where you needed this was closed.

I'm still using the feature but I'm using my patch locally

Do you have the bandwidth to update this PR?

Not right now, so if you feel we should close this PR, we can do it and I may reopen it later.
The patch in itself is tiny and straightforward, but I would need a bit of spare time to write the tests

If not, I can maybe create an issue based on the PR and eventually someone else can pick this up. What you're fixing is basically a bug, as binary resources should be supported.

There is already an issue, #1000

@dimaqq dimaqq added the hint/help-wanted Extra attention is needed label Nov 26, 2024
@dimaqq dimaqq force-pushed the allow-bin-resource branch 2 times, most recently from ccddb5e to d165ea5 Compare November 26, 2024 05:02
@dimaqq
Copy link
Contributor

dimaqq commented Nov 26, 2024

  • rebased
  • fixed the yaml code path
  • added type hints

@dimaqq
Copy link
Contributor

dimaqq commented Nov 26, 2024

Moved to #1218

@dimaqq dimaqq closed this Nov 26, 2024
jujubot added a commit that referenced this pull request Nov 27, 2024
#1218

I've reset author and re-signed commits from #1147 to get the CLA check to pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hint/help-wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to upload .tar file as local resource to deployed charmed application
4 participants